Repository navigation
Add string sample logs - #116
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
Missing-value filters can return all data, and transition timestamps are incorrectly included.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Adds string sample-log filtering, validation, and corrected filter handling in histogram/output generation.
Changes:
- Supports case-insensitive string predicates and HDF5 string logs.
- Sorts and applies include/exclude ranges correctly.
- Adds tests, plotting support, and documentation.
File summaries
| File | Description |
|---|---|
src/test_utils.rs |
Adds mock sample-log creation. |
src/stats.rs |
Propagates log-filter errors. |
src/interface.rs |
Exposes string filtering. |
src/filters/mod.rs |
Exports predicates. |
src/filters/api.rs |
Adds string predicates and errors. |
src/data/save/sample_logs.rs |
Saves and filters string logs. |
src/data/sample_logs.rs |
Loads, filters, and sorts log ranges. |
src/data/nexus_data.rs |
Returns string logs to Python. |
src/batch_interface.rs |
Validates and applies string filters. |
MNeuEventLib/plotting.py |
Plots categorical logs. |
docs/source/how-to/filtering.ipynb |
Documents text filtering. |
Review details
- Files reviewed: 11/11 changed files
- Comments generated: 2
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
👋 Hi, @alexhroom, Conflicts have been detected against the base branch. Please rebase your branch against the base branch. This message is automatically generated by prince-chrismc/label-merge-conflicts-action so don't hesitate to report issues/improvements there. |
41cb2e9 to
f70f6ad
Compare
|
👋 Hi, @alexhroom, Conflicts have been detected against the base branch. Please rebase your branch against the base branch. This message is automatically generated by prince-chrismc/label-merge-conflicts-action so don't hesitate to report issues/improvements there. |
f70f6ad to
ba100d0
Compare
github-actions
Bot
added
Has Conflicts
|
👋 Hi, @alexhroom, Conflicts have been detected against the base branch. Please rebase your branch against the base branch. This message is automatically generated by prince-chrismc/label-merge-conflicts-action so don't hesitate to report issues/improvements there. |
ba100d0 to
0ad6b9d
Compare
|
👋 Hi, @alexhroom, Conflicts have been detected against the base branch. Please rebase your branch against the base branch. This message is automatically generated by prince-chrismc/label-merge-conflicts-action so don't hesitate to report issues/improvements there. |
0ad6b9d to
6a9bcdb
Compare
| } | ||
| } else if !matches { | ||
| // the run ends where the next value takes over, not at the previous sample | ||
| ends.push(self.time[index].to_ns()); |
There was a problem hiding this comment.
should it be index + 1?
There was a problem hiding this comment.
no, index is currently the index where it no longer matches; so if our log has 2 states, ON or OFF
OFF
ON <- index where start time is
ON
ON
OFF <- index when `!matches` is true, so end time is this time here
OFF
ON
ON
|
👋 Hi, @alexhroom, Conflicts have been detected against the base branch. Please rebase your branch against the base branch. This message is automatically generated by prince-chrismc/label-merge-conflicts-action so don't hesitate to report issues/improvements there. |
6a9bcdb to
7d44f21
Compare
7d44f21 to
5a5271c
Compare
This PR adds the ability to filter sample logs which are strings. Fixes #70.
Also fixes a bug where sample log filters are filtered wrong in output files as
apply_filtersignores whether the time filters are include or exclude, and adds some validation for sample logs when the filter is created rather than when it is calculated.also now sorts sample log times. fixes #100